-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Emit 'connecting' event from PQ handshake #1006
Emit 'connecting' event from PQ handshake #1006
Conversation
322a6b6
to
db6e3e7
Compare
db6e3e7
to
33cf89d
Compare
33cf89d
to
af0152c
Compare
af0152c
to
0da9ad5
Compare
0da9ad5
to
a7d82ca
Compare
a7d82ca
to
3087a62
Compare
8229ed0
to
6b8bcff
Compare
6b8bcff
to
9ff41f7
Compare
9ff41f7
to
5592c18
Compare
5592c18
to
a60d8bc
Compare
Signed-off-by: Mateusz Szczygieł <[email protected]>
a60d8bc
to
74d027a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am missing natlab testcase which checks the happy path where peer receives all expected events when correctly connecting.
I see there's test_pq_vpn_silent_pq_upgrader
however that tests the non-pq server - what about the happy case?
Every test using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
A couple of comments I would like to see fixed in the follow-up PR, but this is good to go in my opinion
Problem
When issuing a PQ connection request, the node state was reported as 'CONNECTING' only when the PQ handshake was finished. This led to no state being reported when the handshake was failing.
Solution
Emit a 'CONNECTING' event upon PQ handshake
To Reviewers
The approach taken here is, to some extent, naive. But unfortunately there is some time pressure to fix the event emiting issue, due to a fact that integrators are, to some extent, blocked by the bug. Therefore fixing of the issue has been divided into two parts, one is implement - maybe not the most clean solution, but quick one to avoid unecessary development blockages. And then refactor this solution into better approach. The quick-and-dirty solution is being implemented in this PR, and the refactoring is being performed here: #1021.
☑️ Definition of Done checklist